fix(evmrpc): return pruned errors from debug_trace* at unavailable heights (PLT-975) - #3888

[suggestion] EnsureTraceHeightAvailable resolves watermarks twice (EnsureBlockHeightAvailable and EnsureStateHeightAvailable each call Watermarks, each of which does a tmClient.Status). On the by-hash path blockByHashRespectingWatermarks adds a third. Since the guard now runs before the semaphore, that is 3 Status calls per request under unbounded concurrency.

Call Watermarks(ctx) once and run the three ensureWithinWatermarks/floor comparisons against that snapshot — it is also more correct, since the current version can mix watermarks from two different reads.

[suggestion] This reverses the invariant that the deleted TestHashBasedTraceEndpointsAcquireSemaphoreBeforeHashLookup (with its panicHashLookupClient) existed to pin: no Tendermint hash lookup before the semaphore is acquired. Two consequences worth stating explicitly rather than leaving implicit in a test rename:

1. BlockByHash + up to 3 Status calls now run outside MaxConcurrentTraceCalls, so that knob no longer bounds the pre-trace work an attacker can drive with debug_traceBlockByHash.
2. The guard now runs on the raw request context, so it is no longer bounded by traceTimeout (prepareTraceContext is what creates that deadline).

Guard-before-wait is the right call for the pruned-height case, so I'm not asking to revert it — but please record the trade-off in the PR body/commit, and consider whether the pre-semaphore lookup needs its own bound.

[nit] This early return is placed ahead of the legacyReceiptFromKVStore fallback, so a below-floor hit no longer consults the legacy KV store. In practice MigrateLegacyReceiptsBatch deletes the legacy key after writing to litt, so the fallback is usually already dead for these hashes — but that makes the interaction worth a word in the comment, and it is the mechanism behind the legacy-receipt amplifier noted on ErrReceiptPruned.

[nit] The nil guard is the right fix for the panic, but it leans on the discarded error one line up. Prefer making the failure explicit — ethtx, err := m.AsTransaction(); if err != nil || ethtx == nil { continue } — so a malformed EVM message is skipped for a stated reason rather than via a nil that reads as accidental.

[nit] Correct fix for the false hit, with a side effect worth noting: a genuinely empty block (no EVM txs) can now never be served from this cache and pays a full trace on every request. tryBlockResultCache still covers it if a whole-block entry was baked, so this is likely acceptable — just confirm empty blocks do get block-level entries, otherwise this is a small permanent regression on a common case.

[suggestion] This branch is unreachable, and the error it returns isn't matchable. blockByHashRespectingWatermarks → blockByHashWithRetry already converts blockRes.Block == nil into ErrBlockNotFoundByHash (evmrpc/utils.go:179), so a (nil-block, nil-error) return can't occur.

More importantly, if it ever did, a bare fmt.Errorf("block %s not found") can't be recognised by callers — the rest of the package keys off the ErrBlockNotFoundByHash sentinel (e.g. blockByHashOrNullForJSONRPC maps it to JSON null). Either drop the branch or return fmt.Errorf("block %s: %w", hash.Hex(), ErrBlockNotFoundByHash).

[suggestion] This early return also skips the legacyReceiptFromKVStore fallback below. Previously a below-floor receipt returned ErrNotFound and the legacy KV store was still consulted; now it short-circuits. The overlap case (a receipt present in litt and below the litt floor and also in legacy KV) should be rare — pre-litt receipts aren't in litt at all — but the ordering change is silent. A brief comment stating that a pruned litt entry is authoritative and deliberately does not fall through to legacy would pin the intent.

[suggestion] A single guarded trace call now recomputes Watermarks up to three times: once via latestTraceHeight → LatestHeight, once in EnsureBlockHeightAvailable, once in EnsureStateHeightAvailable. Each does a tmClient.Status round trip plus receipt/state version reads, and — since the guard was deliberately moved ahead of the semaphore — this is now unbounded under trace load.

Fetching once and applying the three comparisons inline would be both cheaper and more consistent (today the block and state legs can observe different latest values mid-flight):

blockEarliest, stateEarliest, latest, err := m.Watermarks(ctx)
if err != nil { return err }
if err := ensureWithinWatermarks(height, blockEarliest, latest); err != nil { return err }
if err := m.EnsureReceiptHeightAvailable(height); err != nil { return err }
// ... state leg against stateEarliest/latest

This also reads better against the "guard at the choke point" rule in AGENTS.md: one watermark snapshot, three named checks.

[nit] Worth a comment on why an empty tx list must not be a hit. As written this reads like a perf regression — an empty block used to short-circuit to [] and now walks the full trace path to produce the same []. The actual reason is that the old code reported a hit without ever consulting the cache, which let an unavailable height answer [] with HTTP 200 (the silent-[] row in your PR table). Naming that in the comment stops someone reverting it as a pointless miss.

[nit] The guard fixes a real nil-deref, but pairing it with the discarded error on the line above makes a malformed message vanish with no trace. Since you're already touching this, consider ethtx, err := m.AsTransaction(); if err != nil || ethtx == nil { continue } — same control flow, but the error is at least available to log or count if this ever fires.

[suggestion] This reverses an invariant that was previously pinned on purpose. The deleted TestHashBasedTraceEndpointsAcquireSemaphoreBeforeHashLookup used a client whose BlockByHash panicked with "hash lookup should not happen before trace context setup", i.e. hash resolution was deliberately deferred until after the MaxConcurrentTraceCalls semaphore. After this change every debug_traceBlockByHash / debug_traceCall (and, via latestTraceHeight, every by-number trace) performs a tmClient.Status plus a Tendermint BlockByHash lookup outside the concurrency limit.

The trade-off looks defensible — a pruned request shouldn't have to win a semaphore slot to learn it's pruned, and these are cheap in-process reads next to an actual trace — but the PR description frames it only as "reorder so guard precedes prepareTraceContext" and doesn't mention that a pinned protection was removed. Worth stating the reasoning explicitly here or in the description, since the next reader will find the deleted test in history and not know it was intentional.

[suggestion] This early return skips the legacy KV fallback below. Previously a below-floor hit surfaced as ErrNotFound from GetReceiptFromStore and fell through to legacyReceiptFromKVStore; now it errors out immediately. For a node whose legacy KV store still holds receipts for a height that litt has aged past, that's a served receipt turning into an error.

The overlap is probably empty in practice (legacy receipts predate litt, so they shouldn't have litt entries at all), which is why I'm not calling it blocking — but the safer ordering is to attempt legacyReceiptFromKVStore first and only return the ErrReceiptPruned wrap if that also misses. That keeps "pruned" meaning "unavailable everywhere", which is what the trace guard actually wants to assert.

[suggestion] The stateStore == nil short-circuit and its comment are duplicated verbatim in EnsureTraceHeightAvailable (line 221). AGENTS.md's "guard at the choke point, never at each caller" applies: a third trace guard added later has to remember this, and the identical comment in two places is the tell. Hoisting it into one named helper — something like ensureReplayStateAvailable(ctx, height) whose doc comment carries the why (SS disabled ⇒ replay reads state via SC/ctxProvider, so SS watermarks don't apply) — would leave both Ensure* methods reading as a clean sequence of steps.

Related: EnsureStateHeightAvailable is exported and, with stateStore == nil, Watermarks() sets stateEarliest = latest, so it reports every historical height as pruned — pinned by TestEnsureStateHeightAvailable's "nil state store" subtest. That's a trap for a future caller who reaches for it directly. Worth a doc-comment sentence saying it reports SS retention only and is not meaningful when SS is disabled.

[suggestion] Two things about the new guard layer:

1. guardTraceRequest{,ByNumber,ByHash,ByNumberOrHash,ByTxHash} and guardTraceCallRequest{,ByNumber,ByHash,ByNumberOrHash} are nine functions where the by-number/by-hash/by-number-or-hash trios are byte-for-byte identical apart from which Ensure* method the leaf calls. Threading the availability check through instead — e.g. one family taking ensure func(context.Context, int64) error, with EnsureTraceHeightAvailable / EnsureTraceCallHeightAvailable passed at the entry points — would halve this without losing the replay-vs-call distinction the PR is careful to draw.

2. Ordering side effect: the watermark check now runs before guardHistoricalDebugTraceHeight, so recordHistoricalDebugTraceAttempt no longer fires for a height that is both pruned and beyond maxBlockLookback. If that metric is used to size MaxTraceLookbackBlocks, it now undercounts on pruning-heavy nodes. Probably fine, but it's a silent observability change not mentioned in the description.

[nit] The what is clear but the why is the load-bearing part and it's only in the test name. The reason an empty txHashes must not be a hit is that Backend.BlockByNumber drops txs whose receipts aren't found (if !found { continue }), so an empty list is indistinguishable from "receipts pruned" — which is exactly the silent []/HTTP 200 failure mode in the PR's table. Per AGENTS.md, that belongs in the doc comment above (currently just "assembles a per-tx hit; returns (nil, false) if any miss").

Also worth a word that a genuinely empty block now always falls through to the full trace path; harmless (no txs to replay, and tryBlockResultCache still covers the block-level entry), but it reads like an oversight without the note.

[nit] Swapping ConvertBlockNumber for getBlockNumber also changes pending handling: the old code did panic("tracing on pending block is not supported"), while getBlockNumber maps PendingBlockNumber to nil (= latest). resolveDebugTraceBlockNumber maps it to latestTraceHeight too, so guard and execution stay consistent and this is a strict improvement over panicking — but it's a user-visible semantic change (debug_traceBlockByNumber("pending") now traces latest instead of erroring) that the description doesn't list. Worth a line in the PR body, since it's the kind of thing an integrator notices before we do.

[nit] rs.earliest = 150 is a no-op — rs is constructed with earliest: 150 five lines up. The comment above ("Receipts pruned below 150") reads as though this line establishes that state, so a later reader may adjust the constructor and assume this line re-pins it. Either drop the assignment or initialise rs with earliest: 1 so the mutation is what actually moves the floor (as TestEnsureTraceHeightAvailable at line 192 does).

Lookback uses tip, not safe latest

Medium Severity

resolveDebugTraceBlockNumber now resolves latest via latestTraceHeight (watermark-safe), but guardHistoricalDebugTraceHeight still measures lookback against the raw app tip. When tip outruns the safe latest and maxBlockLookback is small (especially 0), the safe height is treated as historical and common debug_trace* requests fail even though watermark checks would allow them.

 

^{Reviewed by Cursor Bugbot for commit a558ab6. Configure here.}

[suggestion] This checks api.backend == nil || api.tmClient == nil but not api.backend.watermarks == nil, unlike guardTraceRequest (line 106) and guardTraceCallRequest (line 174), which both guard the nil watermark manager explicitly. If watermarks is nil, blockByHashRespectingWatermarks returns errNoHeightSource outright (watermark_manager.go:267), so debug_traceBlockByHash hard-fails with "unable to determine height information" where the old code skipped the guard and proceeded.

Same applies to guardTraceCallRequestByHash at line 191. Production always wires a non-nil manager, so this is robustness/consistency rather than a live bug — but the file's own convention is to nil-check it, and doing so here keeps the two hash guards degrading the same way as the height guards.

[suggestion] Worth double-checking the freshly-landed-tx path: the receipt's exact BlockNumber is fed to EnsureTraceHeightAvailable, whose EnsureBlockHeightAvailable leg compares against the composite safe latest = min(tmLatest, ctxTip, receiptStore.LatestVersion(), stateStore.GetLatestVersion()). The receipt being readable implies the first three are at or above that height, but the SS latest can lag. If it does, tracing a tx in the newest block now returns requested height N is not yet available — a transient rejection on the hottest trace path.

latestTraceHeight (line 132) exists precisely to absorb that skew, but it only covers the latest/safe/finalized tag path, not this one where the height comes from the receipt. If SS commit is synchronous with the receipt write this is a non-issue; if it isn't, consider clamping to the safe latest (or treating ErrBlockHeightNotYetAvailable as non-fatal) for a receipt-confirmed height.

[suggestion] "never reads receipts" overstates what this guard achieves, and Codex flags the same thing. The receipt floor is indeed skipped, but EnsureBlockHeightAvailable derives latest from Watermarks, where latest = min(tmLatest, ctxTip, m.receiptStore.LatestVersion(), ...). So a receipt store lagging at the tip still rejects an explicit height whose block and state are both present, and still drags the latest tag to an older block via latestTraceHeight.

Using the shared safe-latest is consistent with the rest of evmrpc (ResolveHeight, blockByNumberRespectingWatermarks) and is probably the intended conservative choice — but the doc comment reads as a stronger guarantee than the code gives. Suggest narrowing it to something like "skips the receipt retention floor; the shared safe-latest ceiling still applies."

[nit] EnsureTraceHeightAvailable calls Watermarks twice (once via EnsureBlockHeightAvailable, once via EnsureStateHeightAvailable), and callers reaching it through guardTraceRequestByNumber → latestTraceHeight add a third; guardTraceRequestByHash adds a fourth through blockByHashRespectingWatermarks. Each call is a tmClient.Status() plus store version loads, and they can observe different snapshots mid-flight, so the block and state legs aren't necessarily checked against the same latest.

Resolving blockEarliest, stateEarliest, latest once here and calling ensureWithinWatermarks twice against that snapshot would be both cheaper and internally consistent.

[suggestion] This early return skips the legacyReceiptFromKVStore fallback that a below-floor receipt previously reached (it used to return ErrNotFound and fall through at line 250). During a migration window where a receipt exists in both litt and the legacy KV store, a litt retention floor above that block will now hide a receipt the legacy store can still serve.

That may well be intended — pruned is pruned, and answering from legacy would make the retention floor a lie. But it is an unstated behavior change in a shared store method used well beyond the trace guards; worth a one-line comment recording the choice.

[nit] The nil check fixes the panic, but the error from AsTransaction() is still discarded on line 231, so a message that fails to decode is now silently dropped from block results instead of crashing. Per the Consistency section of evmrpc/AGENTS.md ("RPC responses for historical heights should never change"), a transaction quietly vanishing from eth_getBlockByNumber / tx counts is the kind of thing you want a trace of. Capturing the error and logging it at warn would make the drop diagnosable without changing the response.

[blocker] Flooring the parent at 0 makes tracing the chain's first block unconditionally fail. For height == genesisInitialHeight (1 on a normal chain) this computes parentBlockHeight = 0, and EnsureBlockHeightAvailable(0) runs ensureWithinWatermarks(0, blockEarliest, latest) with blockEarliest = 1, returning requested height 0 has been pruned; earliest available is 1.

So on a full-history node debug_traceBlockByNumber("earliest") — which resolveDebugTraceBlockNumber explicitly resolves to Genesis.InitialHeight — and debug_traceBlockByNumber(0x1) both now error out where they previously worked. The actual replay does not need a parent block: initializeBlock only calls tmClient.Validators(ctx, &prevBlockHeight, ...), and Tendermint treats height 0 as "latest", so the call it is guarding would have succeeded.

The state leg two lines above already handles this correctly with max(height-1, m.genesisInitialHeight()); the same floor belongs here:

parentBlockHeight := max(height-1, m.genesisInitialHeight())

The guard is right for the pruned/state-synced case (tracing at blockEarliest on a snapshot node genuinely cannot load blockEarliest-1 validators) — it is only the genesis edge that over-rejects. Note TestEnsureTraceHeightAvailableParentBlockFloor currently pins the over-strict behaviour at the block floor, so it will need the genesis case distinguished from the pruned case.

[suggestion] The m.stateStore == nil short-circuit is repeated here and again in EnsureTraceHeightAvailable (line 225), with the same comment both times. AGENTS.md asks for the guard at the choke point rather than at each caller — EnsureStateHeightAvailable is that choke point, and every caller of it needs this branch.

As it stands EnsureStateHeightAvailable has a surprising standalone contract when SS is disabled: Watermarks sets stateEarliest = latest, so any historical height reports "has been pruned" (your own TestEnsureStateHeightAvailable subtest pins exactly that). A third caller added later gets the wrong answer unless they remember to repeat the check. Folding the nil-store case into EnsureStateHeightAvailable — returning nil, with the SC/ctxProvider rationale in its doc comment — makes it an invariant instead of a convention.

[suggestion] This nil-check is narrower than the one in guardTraceRequest, which tests api.backend != nil && api.backend.watermarks != nil. If backend is non-nil but backend.watermarks is nil, blockByHashRespectingWatermarks returns errNoHeightSource ("unable to determine height information"), and since this rewrite propagates errors instead of falling through, debug_traceBlockByHash hard-fails on a node in that state — previously it returned nil and tracing proceeded.

Same in guardTraceCallRequestByHash (line 191). Extending the condition to api.backend == nil || api.backend.watermarks == nil || api.tmClient == nil keeps the degraded-configuration behaviour aligned with guardTraceRequest.

[nit] The equivalent nil-guard added in simulate.go carries // AsTransaction may return nil if it fails to unpack the tx data. — worth repeating here, since ethtx, _ := m.AsTransaction() discards the error and the bare continue gives a reader nothing to go on.